Skip to content

Introduce TagStore for eager tag post-processors (dense store - part 3) - #13001

Open
dougqh wants to merge 28 commits into
masterfrom
dougqh/eager-postprocessors-span-context
Open

dougqh wants to merge 28 commits into
masterfrom
dougqh/eager-postprocessors-span-context

Conversation

@dougqh

@dougqh dougqh commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

What Does This Do

Introduces TagStore, a span's tags as stored, and runs the eager tag post-processors against it instead of a TagMap.

  • TagStore has load, loadString and store, each by name or by known tag id, plus store(EntryReader). Loads and stores go straight to storage, with no interception or interpretation, as Map was to TagMap. A null value removes the tag, and a null entry is a no-op. Thread safety is left to each implementation.
  • Known tags by id. A known tag's KnownTags id and its name address the same tag. The id skips classifying the name on every call, and after Have the span own its tag stores directly (dense store - part 7) #13003 it's a direct lookup in the span's id store. Custom tags have no id.
  • Its own verbs. load/store are distinct from TagMap's get/set and the span's intercepted getTag/setTag. One type can offer the raw and the intercepted operations without a name clash, and a call site's verb says which one it means.
  • DDSpanContext implements TagStore, but a span isn't conceptually one. The store is the storage beneath the span. The span implements it only so post-processing can be handed that storage without allocating: a wrapper would be free only if escape analysis removed it, and the processor chain's dispatch loop prevents that. Its methods are unsynchronized, because callers already hold the span's tag lock, as earlyProcessTags does.
  • TagMap implements it too, through its own get and set, so standalone maps and tests can be passed directly.
  • The eager processors switch to EagerTagsPostProcessor, which takes a TagStore. They are PeerServiceCalculator, InternalTagsAdder and HttpEndpointPostProcessor, chained by EagerPostProcessorChain. NamingSchema.ForPeerService and EndpointResolver take TagStore too.
  • They address their tags by id. Every tag they touch is a known tag. The peer service precursor lists stay names, since they include tags outside the registry, such as hazelcast.instance.
  • OtelHttpSemantics.urlOrPath(TagStore). Master's OpenTelemetry HTTP semantics (Emit OpenTelemetry semantic conventions on HTTP spans #12765) made HttpEndpointPostProcessor read the URL through urlOrPath, falling back to url.path. The processor now gets a TagStore, so urlOrPath gains an overload that reads by id with the same fallback.
  • Why TagStore, not SpanStorage. Everything on it is a tag, and TagMap implements it too. Span-named types stay the layer with more to them: SpanData (Take the span's tag lock once in setTagsFrom (tag api - part 1) #13030) is the intercepted view, with span-level fields as well as tags.

Motivation

The eager post-processors run on the application thread as a span finishes. Once a span's tags are no longer one TagMap (#13003), handing them a TagMap means building a view per span on the application thread. In earlier measurements that view cost +4–5% per trace. With TagStore, the span passes itself, and nothing is allocated. That matters because the processor chain loops over an array, so its call sites are megamorphic and any adapter object handed down it would escape.

Additional Notes

  • No behavior change, and no allocation change: the probe gives identical bytes at every step of the trace, 7,936 B per trace both before and after.
  • Tests: internal-api passes, as do the dd-trace-core post-processor, naming, endpoint, DDSpanContext and DDSpan suites. EndpointResolverTest now uses TagMap instead of HashMap.
  • InternalTagsAdder's prebuilt TagMap.Entry objects are a stop-gap. They avoid an Entry per span until the dense store lands (Add a dense known-tag store behind an experimental flag (dense store - part 8) #13004, on by default in Presize dense tag stores and seed span prototypes in bulk (dense store - part 9) #12926). Once it does, the processor can store plain values by id, and the prebuilt entries go. A broader cleanup of the post-processors, including some dead null checks in InternalTagsAdder, is planned separately.
  • The id variants don't change allocation either: TraceShapeBenchmark (children=5, a 7-span trace) gives 7984.0 B/op (legacy) and 7312.0 B/op (newApi, normal) with names and with ids.
  • TagStoreTest checks, for both TagMap and DDSpanContext, that a tag stored by id loads by name and vice versa, that loadString by id converts, and that a null stored by id removes the tag. It passes at every step of the stack, including with the dense store on (Presize dense tag stores and seed span prototypes in bulk (dense store - part 9) #12926).
  • OtelHttpSemanticsTest checks that urlOrPath answers the same from a TagStore as from a map, for each of its URL and path scenarios.

Where this is going

This PR is one step in replacing the span's single TagMap with storage that knows each tag's kind. The goal is to cut what every span allocates on the application thread, without changing behavior.

The end state

Later, not in this stack: cheaper read-through to parent spans, trace-level tags in a per-trace holder (TraceSegment), and primitive tag values without boxing.

The stack. This replaces #12918, split into pieces that can each be reviewed, and measured, on their own.

PR Step Allocation per 7-span trace (flag off unless noted)
#12999 A null tag value removes the tag in TagMap. This is the stack's only behavior change, kept separate on purpose. unchanged
#12992 Bucket known tags by slot in TagMap, and add TraceShapeBenchmark −96 B vs master
#13001 TagStore for the eager post-processors unchanged
#13002 Extract TagMap's bucket mechanics into HashSupport (a pure move) unchanged
#13006 Add the id and name tag stores, not yet used unchanged
#13007 Make TagMap a facade over the two stores unchanged
#13003 Have the span own its tag stores directly +104 B (+1.3%)
#13004 Dense id store behind trace.experimental.dense.tags.enabled (off by default) flag on: −30.6%
#12926 Turn the dense store on by default, add sizing hints and bulk prototype seeding flag on: about 3% less again

Every PR after #12999 is behavior-preserving.

The pieces' allocation figures come from a deterministic per-thread probe (ThreadMXBean allocated bytes) that builds the same 7-span trace as TraceShapeBenchmark. The #12992 and #12926 figures come from TraceShapeBenchmark itself.

🤖 Generated with Claude Code

dougqh and others added 7 commits October 22, 2026 09:31
TagMap bucketed a known tag by folding its serial, so which tags shared
one of the 16 buckets was luck, and each shared bucket costs a
BucketGroup (48 B). Slot coloring already gives tags that appear
together on a span type distinct slots, so bucketing a known tag by its
slot keeps them in distinct buckets more often. Trace-level tags reuse
span-level slot numbers, so the level bit moves them to the other half;
known tags without a slot and custom tags keep their hashes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SpanScenarioBenchmark builds spans and traces the way today's decorators
do (prototype in afterStart, then request or connection tags by name):
bare spans, a server span, the server span via the builder, a jdbc span,
and a server trace with 1, 5 or 20 jdbc children. TraceShapeBenchmark
builds one web-request-shaped trace three ways: every tag by name, the
decorators' calls, and the newest span APIs.

Both finish what they start, so allocation per op covers whole spans and
traces. Single-span results can hide per-child costs that only the trace
scenarios show.

TraceShapeBenchmark is the same file #12918 adds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TraceShapeBenchmark takes the number of jdbc children (1, 5, 20), so
each arm shows how a trace's cost grows with its span count: single-span
and small-trace results can hide costs that land on every child span.
It covers what SpanScenarioBenchmark's trace scenario added, and the
rest of that benchmark overlapped SpanCreationBenchmark (same scenario
names, different semantics), so SpanScenarioBenchmark goes.

TagMapBucketHashTest pins down the bucket hash: known tags get distinct,
nonzero hashes, a slotted tag lands in its slot's bucket (trace-level in
the other half), and tags without a slot and custom tags keep their
hashes. The inline comment now says the level bit flips bit 3.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HeaderPathBenchmark runs one request per op through the propagation
paths: extract the inbound headers with the default propagator, start
the server span from what was extracted, run a few children, inject
into an outbound call, finish everything. Inbound headers vary: none,
Datadog, W3C (with dd, OpenTelemetry and third-party tracestate
members), both, and both plus baggage.

The existing propagation benchmarks each measure one step, so they
can't see costs the extracted state causes later on every span; this
one can. A setup check fails the run if nothing is extracted or
injected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
setTag(tag, null) already removes a tag from a span, but TagMap's own
setters stored a null-valued entry. Internal writers that go straight
to a TagMap could therefore leave null tags on a span, and the
serializer sent them as a msgpack nil under meta.

TagMap.put / set / getAndSet (by name or id, and putAll / fromMap
through set) now remove the tag on a null value and return its prior
value. A null Entry or reader is still a no-op.

The one writer that relied on null tags was payload tagging: a JSON null
in a request or response body produced a tag with a null value. It now
produces no tag, so such a value also no longer counts toward the
max-tags limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kets

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

🎯 Code Coverage (details)
• Patch Coverage: 74.07%
• Overall Coverage: 59.47% (+0.01%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: cdfc195 | Docs | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.81 s 14.70 s [-0.2%; +1.7%] (no difference)
startup:insecure-bank:tracing:Agent 13.56 s 13.68 s [-1.7%; -0.1%] (maybe better)
startup:petclinic:appsec:Agent 17.71 s 17.63 s [-0.4%; +1.3%] (no difference)
startup:petclinic:iast:Agent 17.57 s 17.63 s [-1.1%; +0.4%] (no difference)
startup:petclinic:profiling:Agent 17.33 s 17.30 s [-0.8%; +1.2%] (no difference)
startup:petclinic:sca:Agent 17.84 s 17.70 s [-0.1%; +1.7%] (no difference)
startup:petclinic:tracing:Agent 16.57 s 16.70 s [-1.7%; +0.2%] (no difference)

Commit: cdfc195b · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@dougqh dougqh changed the title Introduce SpanContents for eager tag post-processors Introduce SpanContents for eager tag post-processors (dense store - part 3) Oct 23, 2026
@bric3
bric3 added this pull request to stack #13019 October 23, 2026 16:02
Eager post-processors run on the application thread as a span finishes.
They now take a SpanStorage (a span's tags as stored) instead of a
TagMap, and DDSpanContext implements SpanStorage directly, so no TagMap
view needs to be built for them once span storage stops being a single
TagMap.

SpanStorage has its own verbs, load and store, apart from TagMap's
get/set and the span's intercepted getTag/setTag, so DDSpanContext can
offer the raw and the intercepted operations without a clash. Stores
bypass interception; thread safety is left to each implementation
(DDSpanContext's are unsynchronized, called under the tag lock). TagMap
implements it too, through its own get and set, for standalone maps.

This is the first slice: load, loadString and two stores.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh force-pushed the dougqh/eager-postprocessors-span-context branch from 195841c to 8e57e87 Compare October 23, 2026 16:57
@dougqh dougqh changed the title Introduce SpanContents for eager tag post-processors (dense store - part 3) Introduce SpanStorage for eager tag post-processors (dense store - part 3) Oct 23, 2026
dougqh and others added 2 commits October 26, 2026 11:49
Context.attach() and Context.current() go to the global context manager,
which a standalone JMH run never wires to the benchmark's CoreTracer. The
JDBC and client spans became separate root traces instead of children of
the extracted server span. Use the tracer's attach/currentContext, and
check at setup that the outbound call continues the extracted trace.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* @param spanContext the span, for its other properties
* @param spanLinks the span's links
*/
public abstract void processTags(

@dougqh dougqh Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR, DDSpanContext becomes TagStore, but I think we still want to keep them conceptually separate. I made that change because TagStore is direct to the underlying store without locking or extra interpretation -- just like Map & TagMap before.

The main reason that DDSpanComponent implements TagStore directly is to avoid allocation. Ideally, I think EA (escape analysis)-friendly wrapper could have been used, but the EagerPostProcessorChain dispatch loop isn't EA-friendly.

Comment thread internal-api/src/main/java/datadog/trace/api/TagStore.java Outdated
dougqh and others added 9 commits October 26, 2026 15:42
Everything on it is a tag, and TagMap implements it too, so TagStore says
what it is. Span-named types stay the layer with more to them: SpanData is
the intercepted view, with span-level fields as well as tags.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Sarah Chen <sarah.chen@datadoghq.com>
A span isn't conceptually a TagStore: the store is the storage beneath it,
as a Map was beneath a TagMap. It implements one only so post-processing
can be handed its storage without allocating, since the processor chain's
dispatch loop keeps escape analysis from removing a wrapper. Drop the
TagStore Javadoc's note about this being a first slice.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ssors-span-context

# Conflicts:
#	dd-trace-core/src/main/java/datadog/trace/core/tagprocessor/HttpEndpointPostProcessor.java
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh dougqh changed the title Introduce SpanStorage for eager tag post-processors (dense store - part 3) Introduce TagStore for eager tag post-processors (dense store - part 3) Oct 26, 2026
public InternalTagsAdder(@Nonnull final String ddService, @Nullable final String version) {
this.ddService = UTF8BytesString.create(ddService);
this.baseServiceEntry = TagMap.Entry.create(DDTags.BASE_SERVICE, this.ddService);
this.baseServiceEntry = TagMap.Entry.create(DD_BASE_SERVICE_ID, this.ddService);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once dense store is on by default, we can drop the TagMap.Entry creation in the post processors.

@dougqh
dougqh marked this pull request as ready for review October 26, 2026 20:35
@dougqh
dougqh requested review from a team as code owners October 26, 2026 20:35
@dougqh
dougqh requested review from mhlidd and removed request for a team October 26, 2026 20:35
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T20:37:26.517873Z 36bbbd2 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

dougqh and others added 2 commits October 26, 2026 16:43
EagerTagsPostProcessor and EagerPostProcessorChain still called it
storage after the rename to TagStore, while every processor calls it
unsafeTags.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@datadog-prod-us1-6 datadog-prod-us1-6 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

Static review supports preservation of eager processor ordering, caller-held locking, tag addressing, and HTTP URL-to-path fallback behavior. The completed specialist returned no reportable findings.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit 36bbbd2

@dougqh
dougqh requested review from PerfectSlayer and bric3 October 27, 2026 16:42
Base automatically changed from dougqh/tagmap-slot-buckets to master October 27, 2026 17:39

@datadog-prod-us1-6 datadog-prod-us1-6 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

TagStore preserves the eager processors’ raw-storage access, existing tag-lock boundary, and HTTP URL-to-path fallback. The completed static review reported no actionable defects.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit 3eed220

Comment on lines +5 to +10
* to storage, with no interception or interpretation. Thread safety is up to the implementation;
* {@code DDSpanContext}'s expects the span's tag lock to be held.
*
* <p>Its own verbs, {@code load} and {@code store}, keep it apart from {@link TagMap}'s {@code
* get}/{@code set} and the span's intercepted {@code getTag}/{@code setTag}, so a type can offer
* several of these without a clash.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ question: ‏Why did the Javadoc leaks implementation details from another module?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, fixed in cdfc195. The interface now only says thread safety is up to the implementation. DDSpanContext's implementation already documents that its callers hold the span's tag lock.

Reply written by Claude (Claude Code) on behalf of dougqh.

Comment on lines +17 to +26
Object load(String tag);

/** The tag's value as a string, or null when it's absent. */
String loadString(String tag);

/** The known tag's value, or null when it's absent. */
Object load(long tagId);

/** The known tag's value as a string, or null when it's absent. */
String loadString(long tagId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 suggestion: ‏I would document and annotate nullable return values

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cdfc195: the four load* methods are @Nullable, and so are the values passed to store.

Reply written by Claude (Claude Code) on behalf of dougqh.

void store(long tagId, Object value);

/** Stores the entry's tag; a null entry is a no-op. */
void store(TagMap.EntryReader entry);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ question: ‏Does null reader make sense?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's deliberate, to match how DDSpan.setTag works. TagMap.Entry.create returns null for a null or empty value, and the intent is that null flows through safely, so a caller can write store(Entry.create(id, value)) without its own check. Whether that convention should carry down into TagStore is debatable, though. For now cdfc195 marks the parameter @Nullable so the contract is explicit.

Reply written by Claude (Claude Code) on behalf of dougqh.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up now that this is settled: the rule is that the caller-facing levels (DDSpan.setTag, TagMap, TagStore) accept null, and the storage level doesn't. A null value means the tag is absent, so it removes. A null entry names no tag and no id, so a no-op is the only thing it can mean. One level down, #13006 (dense store part 5) makes HashStore/IdStore reject nulls on writes (0291032), so null is handled once, by their owner.

Reply written by Claude (Claude Code) on behalf of dougqh.

String loadString(long tagId);

/** Stores the tag; a null value removes it. */
void store(String tag, Object value);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ question: ‏Is there a rationale behind allowing null value for removal rather than dedicated remove method? Won't we allow unexpected code path doing that? (caller that did not know they had null values removing the existing oner rather than updating)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It matches TagMap.set and a span's setTag(tag, null), both of which remove. It also can't produce the path you describe: load returns null for an absent tag, so a tag holding a null value can't be represented. "Updated to null" and "removed" leave the store in the same observable state. I'd rather keep the interface small, but I can add an explicit remove if you think it reads better at call sites.

Reply written by Claude (Claude Code) on behalf of dougqh.

private final EagerTagsPostProcessor[] chain;

public EagerPostProcessorChain(@Nonnull final EagerTagsPostProcessor... processors) {
chain = Objects.requireNonNull(processors);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ question: ‏Does it make sense to forbid null but allow empty array?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It mirrors the existing PostProcessorChain on master. An empty chain is a valid no-op (no processors enabled), while a null array is a programming error. In practice it's never empty: TagsPostProcessorFactory always adds PeerServiceCalculator.

Reply written by Claude (Claude Code) on behalf of dougqh.

…ut of its Javadoc

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants